Skip to content

GhostTree new logic level insane, take a cup of java before reviewing this #3334

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

GhostTree new logic level insane, take a cup of java before reviewing this #3334

wants to merge 10 commits into from

Conversation

Hypernoot
Copy link
Member

@Hypernoot Hypernoot commented Aug 11, 2025 •
edited
Loading

see #3217

@Hypernoot Hypernoot marked this pull request as draft August 11, 2025 17:20
@Hypernoot Hypernoot marked this pull request as ready for review September 6, 2025 16:16
@Hypernoot Hypernoot changed the title WIP GhostTree GhostTree new logic level insane, take a cup of java before reviewing this Sep 6, 2025
@tobbi
Copy link
Member

tobbi commented Sep 6, 2025 

src/badguy/ghosttree_attack.cpp:340:14: style: Variable 'root' is assigned a value that is never used. [unreadVariable]
  auto& root = Sector::get().add<GhostTreeRootMain>(Vector(p ? p->get_pos().x : m_pos.x, m_pos.y), this);
             ^
src/badguy/ghosttree_attack.cpp:409:14: style: Variable 'root' is assigned a value that is never used. [unreadVariable]
  auto& root = Sector::get().add<GhostTreeRootGreen>(pos, this);
             ^
src/badguy/ghosttree_attack.cpp:437:14: style: Variable 'root' is assigned a value that is never used. [unreadVariable]
  auto& root = Sector::get().add<GhostTreeRootBlue>(pos, this);
             ^
src/badguy/ghosttree_attack.cpp:467:14: style: Variable 'root' is assigned a value that is never used. [unreadVariable]
  auto& root = Sector::get().add<GhostTreeRootPinch>(pos, this);

tobbi
tobbi reviewed Sep 6, 2025
View reviewed changes
src/badguy/ghosttree.cpp Outdated

switch (color) {
case ATTACK_RED:
willowisp.set_color(Color(1, 0, 0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the constants in Color class...

Color::RED, etc.

@tobbi
Copy link
Member

tobbi commented Sep 6, 2025

Can you show a video?

@Hypernoot
Copy link
Member Author

ghosttree.zip
I fixed both, here you have a video.

@tobbi
Copy link
Member

tobbi commented Sep 6, 2025

Not bad. I'd like to see some intermediate frames for the animation.

@tobbi
Copy link
Member

tobbi commented Sep 6, 2025

Also, the mouth opening should be earlier, before the willowisps are moving towards the mouth.

@Hypernoot
Copy link
Member Author

Thanks for your feedback! Intermediate frames would be nice, indeed, but we're unlikely to get them in reasonable time. I was not provided any sucking animation, so that's why it looks this weird. As an alternative solution, the tree could suck the wisp through his eyes. Nevertheless, I would rather wait now for the rest of the team, whether they have any comments to the visuals.

@Hypernoot Hypernoot added this to the 0.7.0 milestone Sep 7, 2025
@Hypernoot Hypernoot mentioned this pull request Sep 7, 2025
Closed
@Frostwithasideofsalt
Copy link
Member

imma test this

@Frostwithasideofsalt
Copy link
Member

(re-post cause i put it under the wrong pr)
seems mostly fine, my only real complains are that the attacks seem slow and are on a oddly high z layer, the hitbox for the orb is slightly too high, and i feel like the ghost tree should open his mount only when inhaling wisps- also i dont really undestand when it's face sorta comes off? it feels kinda random.

also just to mention it before someone else does, it feels really unpolished rn, but that doesnt matter because its very blatantly unfinished and its just important to get the basic behavior down first, and the visual polishing with stuff like particle animations can just be done later or in another pr. So yeah little johnny dont be commenting "why is the ghost tree so bad,,,... or whatever" lol

huntekye
huntekye reviewed Sep 29, 2025
View reviewed changes
Copy link
Contributor

@huntekye huntekye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just looking at the code as-is, it seems good to me; I left a few comments, but you're free to ignore them lol. I didn't try to follow a lot of the higher-level flow; I assume if it plays fine and doesn't crash then things should be fine there. Let me know if you need me to test the build as well.

data/images/creatures/ghosttree/blue_root.sprite
)
(action
(name "flying-right")
(hitbox 11 12 70 8)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason that the shape of the hitbox changes? 11 vs 15?

src/badguy/ghosttree.cpp
if ((mystate == STATE_SUCKING) && (willowisp->was_sucked)) {
mystate = STATE_IDLE;
GhostTree::rotate_willo_color() {
m_next_willo = static_cast<AttackType>((static_cast<int>(m_next_willo) + 1) % 3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be a case / else-if tree to avoid treating enums like integers?

src/badguy/ghosttree.cpp
while (m_willo_to_spawn--) {
spawn_willowisp(m_attack);
}
m_attack = static_cast<AttackType>(static_cast<int>(m_attack) + 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I don't love treating the enum like an integer; I don't mind it that much here, but I think it would be worth adding a comment in the header that the order of the elements in the enum is important (maybe that's why you have ATTACK_RED = 0, idk). On the other hand it just generally feels like a bad idea to be relying on the ordering of the enum.

src/badguy/ghosttree_attack.cpp
m_parent(parent)
{
m_physic.set_velocity_y(-RED_ROOT_SPEED);
const int variant = abs(static_cast<int>(pos.x)) % 3 + 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just RNG?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobbi tobbi tobbi left review comments

+1 more reviewer

@huntekye huntekye huntekye left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
priority:urgent
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Hypernoot @tobbi @Frostwithasideofsalt @huntekye @Rusty-Box